Skip to content

Fix access list enterprise tests.#30925

Merged
mdwn merged 1 commit into
masterfrom
mike.wilson/fix-e-tests
Aug 23, 2023
Merged

Fix access list enterprise tests.#30925
mdwn merged 1 commit into
masterfrom
mike.wilson/fix-e-tests

Conversation

@mdwn
Copy link
Copy Markdown
Contributor

@mdwn mdwn commented Aug 23, 2023

The access list enterprise tests were broken by changing the custom JSON marshaling of the access list audit from map[string]interface{} to map[string]string. This change restores the old behavior.

The access list enterprise tests were broken by changing the custom JSON
marshaling of the access list audit from `map[string]interface{}` to
`map[string]string`. This change restores the old behavior.
@jakule
Copy link
Copy Markdown
Contributor

jakule commented Aug 23, 2023

BTW, wouldn't make more sense to update the tests in e?

@mdwn
Copy link
Copy Markdown
Contributor Author

mdwn commented Aug 23, 2023

BTW, wouldn't make more sense to update the tests in e?

I was originally attempting to fix the tests in e and I was still getting this issue, so I suspect there's something additional in the JSON that we can't necessarily control. IMO this just seems safer.

@mdwn mdwn enabled auto-merge August 23, 2023 19:55
@mdwn mdwn added this pull request to the merge queue Aug 23, 2023
Merged via the queue into master with commit c0a459b Aug 23, 2023
@mdwn mdwn deleted the mike.wilson/fix-e-tests branch August 23, 2023 20:15
@public-teleport-github-review-bot
Copy link
Copy Markdown

@mdwn See the table below for backport results.

Branch Result
branch/v13 Create PR


func (a *Audit) UnmarshalJSON(data []byte) error {
var audit map[string]string
var audit map[string]interface{}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already merged but why don't you just use a temp struct to unmarshal these values. The unmarshal to map[string]any is not good when we know the types and fields.
Extending to other fields isn't trivial and the current form is quite difficult to understand

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to look into that as a follow on PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with this ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants